I can understand not wanting to break installs for existing users, but I'm a little uneasy about introducing a new supply chain risk by relying on a third party to re-upload protoc to crates.io.

I wonder if we could avoid requiring protoc when installing this crate by storing the generated Rust structs in source control. In other projects we do this by overriding the OUT_DIR in build.rs, and then include! the generated file.

let out_dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src");
unsafe { env::set_var("OUT_DIR", &out_dir) };

That can be combined with an envvar check to only compile the protobuf if protoc is installed:

if env::var("PROTOC").is_ok() {
    prost_build::compile_protos(...)?;
} else {
    println!("protoc not found, skipping protobuf generation");
}

I can understand not wanting to break installs for existing users, but I'm a little uneasy about introducing a new supply chain risk by relying on a third party to re-upload protoc to crates.io.

I wonder if we could avoid requiring protoc when installing this crate by storing the generated Rust structs in source control. In other projects we do this by overriding the OUT_DIR in build.rs, and then include! the generated file.

let out_dir = concat!(env!("CARGO_MANIFEST_DIR"), "/src");
unsafe { env::set_var("OUT_DIR", &out_dir) };

That can be combined with an envvar check to only compile the protobuf if protoc is installed:

if env::var("PROTOC").is_ok() {
    prost_build::compile_protos(...)?;
} else {
    println!("protoc not found, skipping protobuf generation");
}

thanks for the review! I removed the dependency.

I am not sure 100% if my solution is what you had in mind though since I simply added pub mod protobuf instead of include! on the generated file.

also added a custom env var check.

Yeah, that's roughly what I was thinking. But it looks like CI needs fixing (unrelated to your change)

Error: This request has been automatically failed because it uses a deprecated version of actions/cache: v2. Please update your workflow to use v3/v4 of actions/cache to avoid interruptions. Learn more: https://github.blog/changelog/2024-12-05-notice-of-upcoming-releases-and-breaking-changes-for-github-actions/#actions-cache-v1-v2-and-actions-toolkit-cache-package-closing-down

I'm not sure where the PROTOC envvar is supposed to come from... I get the feeling that's a new thing that prost-build invented. Ideally we'd check to see if protoc is on the load path. Maybe this approach works?

I'm not sure where the PROTOC envvar is supposed to come from... I get the feeling that's a new thing that prost-build invented. Ideally we'd check to see if protoc is on the load path. Maybe this approach works?

I think it's something they invented. this env var was never set for me. I replaced it with a Command check as suggested.

Looks good! There's just one more failure because of the actions/cache deprecation

Looks good! There's just one more failure because of the actions/cache deprecation

ahh missed that - fixed! 🙏🏼